[Backport 17.03] Adding ipam options to ipam driver requests #2449

fcrisciani · 2017-11-15T23:49:30Z

Backport PR:#2324

fcrisciani · 2017-11-15T23:51:21Z

@nishanttotla @stevvooe @abhi for the review.
NOTE: I had to nuke the api_client_test.mock.go file because has dependencies on types not anymore in docker, I checked master and I did not see the code there, let's see if it actually breaks something maybe some test.

stevvooe · 2017-11-15T23:56:35Z

vendor.conf

@@ -14,18 +14,18 @@ github.com/prometheus/common ebdfc6da46522d58825777cf1f90490a5b1ef1d8
 github.com/prometheus/procfs abf152e5f3e97f2fafac028d2cc06c1feb87ffa5

 github.com/docker/distribution 7230e9def796c63a4033211dc5107742d689fc1e
-github.com/docker/docker 0fb0d67008157add34f1e11685e23a691db92644
+github.com/docker/docker e4512d264741e83e954a19f9ef5e3cb06c5856b6


What version is this coming from?

so the problem is the following:
the original commit does not contain an api that libnetwork uses.
This is the commit id that is used inside libnetwork: https://github.com/docker/libnetwork/blob/release/v0.9/Godeps/Godeps.json#L147

But unfortunately the latter does not have anymore the types: types.ContainersPruneConfig, types.ImagesPruneConfig, types.NetworksPruneConfig, types.VolumesPruneConfig
That got nuked by this commit: moby/moby#28535

stevvooe · 2017-11-15T23:57:06Z

NOTE: I had to nuke the api_client_test.mock.go file because has dependencies on types not anymore in docker, I checked master and I did not see the code there, let's see if it actually breaks something maybe some test.

Yea, that is kind of a problem. It will break the tests. What version of docker are you bringing in?

codecov · 2017-11-16T00:52:54Z

Codecov Report

Merging #2449 into bump_v17.03 will increase coverage by 0.2%.
The diff coverage is 65.45%.

@@              Coverage Diff               @@
##           bump_v17.03    #2449     +/-   ##
==============================================
+ Coverage        55.41%   55.61%   +0.2%     
==============================================
  Files              102      102             
  Lines            21566    21596     +30     
==============================================
+ Hits             11950    12011     +61     
+ Misses            8456     8434     -22     
+ Partials          1160     1151      -9

fcrisciani · 2017-11-16T18:00:42Z

@stevvooe let's see this one how it goes, but basically I researched which was the PR that fixed the API name change in the swarmkit history and was:#1876
so I backported this one too to remove that error.

This seems to be enough to cleanly have a green CI.

fcrisciani · 2017-11-16T18:02:43Z

manager/allocator/networkallocator/networkallocator_test.go

@@ -753,6 +753,10 @@ func (a *mockIpam) DiscoverDelete(dType discoverapi.DiscoveryType, data interfac
 	return nil
 }

+func (a *mockIpam) IsBuiltIn() bool {


This is coming from this PR: 8afb5ec

fcrisciani · 2017-11-16T18:03:07Z

vendor.conf

 github.com/docker/go-connections 34b5052da6b11e27f5f2e357b38b571ddddd3928
 github.com/docker/go-events 37d35add5005832485c0225ec870121b78fcff1c
 github.com/docker/go-units 954fed01cc617c55d838fa2230073f2cb17386c8
 github.com/docker/libkv 9fd56606e928ff1f309808f5d5a0b7a2ef73f9a8
-github.com/docker/libnetwork 3ab699ea36573d98f481d233c30c742ade737565
+github.com/docker/libnetwork 878043960238db64a7e783199f688211560dd84c https://github.com/fcrisciani/libnetwork


temporary till the libnetwork change gets merged

nishanttotla · 2017-11-16T21:08:26Z

manager/allocator/networkallocator/networkallocator.go

+		dOptions = setIPAMSerialAlloc(dOptions)
+		defer delete(dOptions, ipamapi.RequestAddressType)
+
+		gwIP, _, err := ipam.RequestAddress(poolID, net.ParseIP(ic.Gateway), dOptions)


@fcrisciani in the master branch, this statement seems to be inside a conditional:

if ic.Gateway != "" || gwIP == nil { ... }

Do we need that here?

good point, have to check

Looking at it better looks like that logic with the if seems had been introduced by another commit fb74191, that is a major restructure looks like and the logic there extract gwIP.

This change as is does not change the current behavior but simply pass the additional flag to the IPAM.

Sounds good, thanks for checking!

nishanttotla · 2017-11-16T21:10:04Z

@fcrisciani can we add a healthy number of comments here indicating why these changes were done, and a link to this PR? It may turn out to be useful in the future.

fcrisciani · 2017-11-16T21:36:41Z

@nishanttotla the PR backported(#2324) is the one that I put in the top description, in the commits there is also the reference to the master commit that got ported. I can copy paste the same description of the original PR here, or is there also some other information that you consider useful?

nishanttotla · 2017-11-16T22:11:54Z

@fcrisciani I just meant a comment in networkallocator.go in the code either linking to this PR or a short description of this change, and that it diverges from the master branch. Not super critical though, just nice to have.

fcrisciani · 2017-11-16T22:22:02Z

manager/allocator/networkallocator/networkallocator.go

@@ -764,7 +778,15 @@ func (na *NetworkAllocator) allocatePools(n *api.Network) (map[string]string, er
 		}
 		pools[poolIP.String()] = poolID

-		gwIP, _, err := ipam.RequestAddress(poolID, net.ParseIP(ic.Gateway), nil)
+		if dOptions == nil {


// This change is a backport that leverage a new allocation policy offered by the libnetwork IPAM
// This will let the ipam driver allocate IP sequentially differently from before that was the leftmost free bit of the pool.
// The objective is to reduce the occurrence of address already in use error that is due to a non synchronous handling
// of resources made by swarmkit, where the state of container is deleted from raft and
// the resources (like IP) is made available for other tasks to use immediately while the previous owner is
// still shutting down
@nishanttotla does it work something like this?

@fcrisciani looks good!

k updating the patch now then

Signed-off-by: Flavio Crisciani <[email protected]> (cherry picked from commit ed271d4, fe0d7f6)

Signed-off-by: Flavio Crisciani <[email protected]>

- backport of moby#1876 commit: 7541809 commit: c6aacc7 commit: f250807 To handle the type deletion happened in docker/docker and not updated in swarmkit. The docker vendoring was needed due to a dependency on a method requested by libnetwork vendoring Signed-off-by: Flavio Crisciani <[email protected]>

fcrisciani · 2017-11-22T00:18:29Z

@nishanttotla @stevvooe PTAL

anshulpundir · 2018-02-12T18:33:56Z

@fcrisciani close this out ?

nishanttotla

Sorry for this falling by the wayside. LGTM.

anshulpundir · 2018-02-12T18:37:56Z

Is this even needed at this point ? Its been open since Nov

fcrisciani · 2018-02-12T18:47:48Z

@anshulpundir @nishanttotla not worth anymore, 17.03 is going EOL next month

stevvooe reviewed Nov 15, 2017

View reviewed changes

fcrisciani force-pushed the backport_17.03 branch from 3d2e416 to 4f06009 Compare November 16, 2017 00:52

fcrisciani force-pushed the backport_17.03 branch 2 times, most recently from 2119da0 to 134eaac Compare November 16, 2017 17:59

fcrisciani commented Nov 16, 2017

View reviewed changes

nishanttotla reviewed Nov 16, 2017

View reviewed changes

fcrisciani commented Nov 16, 2017

View reviewed changes

Flavio Crisciani added 3 commits November 17, 2017 11:07

Adding ipam options to ipam driver requests

14b4d5e

Signed-off-by: Flavio Crisciani <[email protected]> (cherry picked from commit ed271d4, fe0d7f6)

Libnetwork vendoring for IPAM serialization

e6d0339

Signed-off-by: Flavio Crisciani <[email protected]>

fcrisciani force-pushed the backport_17.03 branch from 134eaac to b97ab63 Compare November 17, 2017 19:08

nishanttotla approved these changes Feb 12, 2018

View reviewed changes

fcrisciani closed this Feb 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Backport 17.03] Adding ipam options to ipam driver requests #2449

[Backport 17.03] Adding ipam options to ipam driver requests #2449

fcrisciani commented Nov 15, 2017

fcrisciani commented Nov 15, 2017

stevvooe Nov 15, 2017

fcrisciani Nov 16, 2017

stevvooe commented Nov 15, 2017

codecov bot commented Nov 16, 2017 •

edited

Loading

fcrisciani commented Nov 16, 2017 •

edited

Loading

fcrisciani Nov 16, 2017

fcrisciani Nov 16, 2017

nishanttotla Nov 16, 2017

fcrisciani Nov 16, 2017

fcrisciani Nov 16, 2017

nishanttotla Nov 16, 2017

nishanttotla commented Nov 16, 2017

fcrisciani commented Nov 16, 2017 •

edited

Loading

nishanttotla commented Nov 16, 2017

fcrisciani Nov 16, 2017

nishanttotla Nov 17, 2017

fcrisciani Nov 17, 2017

fcrisciani commented Nov 22, 2017

anshulpundir commented Feb 12, 2018

nishanttotla left a comment

anshulpundir commented Feb 12, 2018

fcrisciani commented Feb 12, 2018

[Backport 17.03] Adding ipam options to ipam driver requests #2449

[Backport 17.03] Adding ipam options to ipam driver requests #2449

Conversation

fcrisciani commented Nov 15, 2017

fcrisciani commented Nov 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevvooe commented Nov 15, 2017

codecov bot commented Nov 16, 2017 • edited Loading

Codecov Report

fcrisciani commented Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nishanttotla commented Nov 16, 2017

fcrisciani commented Nov 16, 2017 • edited Loading

nishanttotla commented Nov 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fcrisciani commented Nov 22, 2017

anshulpundir commented Feb 12, 2018

nishanttotla left a comment

Choose a reason for hiding this comment

anshulpundir commented Feb 12, 2018

fcrisciani commented Feb 12, 2018

codecov bot commented Nov 16, 2017 •

edited

Loading

fcrisciani commented Nov 16, 2017 •

edited

Loading

fcrisciani commented Nov 16, 2017 •

edited

Loading